Ad test failures #8969
Ad test failures #8969
Conversation
testing/iframe.js
Outdated
| expectMsg = JSON.stringify(expectMsg); | ||
| actualMsg = JSON.stringify(actualMsg); | ||
| element.expectMessageFromParent = (type, callback) => { | ||
| if (typeof type === 'function') { |
There was a problem hiding this comment.
this is so confusing... 😖 why can't weseparatee them to two functions?
There was a problem hiding this comment.
It's argument reordering, so that you could pass a type, callback, or type and callback.
There was a problem hiding this comment.
Yeah, I think it's not very straightforward for people to use in tests. For example I would have to go through the code to figure out what a callback function should look like.
Maybe update the comment for the function description? At least we should document it clearly.
There was a problem hiding this comment.
I like it. Note: I just found one thing, the callback function is only used for testing expectMessageFromParent function. Do we really need it?
test/functional/test-iframe-stub.js
Outdated
| @@ -19,11 +19,12 @@ import { | |||
| expectPostMessage, | |||
| } from '../../testing/iframe'; | |||
|
|
|||
| describe('test-iframe-createIframeWithMessageStub', () => { | |||
| describe.only('test-iframe-createIframeWithMessageStub', () => { | |||
| @@ -216,7 +216,8 @@ describes.sandboxed('amp-ad-network-adsense-impl', {}, () => { | |||
| }); | |||
| // Not using arrow function here because otherwise the way closure behaves | |||
| // prevents me from calling this.timeout(5000). | |||
| it('with multiple slots', function() { | |||
| // TODO(@tdrl, #8965): Make this pass reliably on Travis. | |||
| it.skip('with multiple slots', function() { | |||
There was a problem hiding this comment.
what about we only skip it on old chrome?
There was a problem hiding this comment.
I prefer this to make A4A fix it faster. 😈
| ad3p.buildCallback(); | ||
| ad3p.preconnectCallback(); | ||
| setTimeout(() => { | ||
| return whenFirstVisible.then(() => { |
There was a problem hiding this comment.
is this the reason why the test is flaky? I can't see why
There was a problem hiding this comment.
Wow! that's a good catch!!!
There was a problem hiding this comment.
@jridgewell could you further explain what does "setTimeout is throttled" mean?
There was a problem hiding this comment.
Chrome throttles weirdly. 🤷♂️
testing/iframe.js
Outdated
| win.addEventListener('message', listener); | ||
| }); | ||
| }; | ||
|
|
||
| function returnTrue() { |
testing/iframe.js
Outdated
| return 'type' in data && data.type == type; | ||
| }; | ||
| } else if (!callback) { | ||
| filter = callbackOrType; |
testing/iframe.js
Outdated
| filter = (data) => { | ||
| return 'type' in data && data.type == type; | ||
| }; | ||
| } else if (!callback) { |
jridgewell
force-pushed
the
ad-test-failuers
branch
from
0aa490f
to
c64d63e
Compare
|
LGTM to @lannka |
jridgewell
force-pushed
the
ad-test-failuers
branch
from
c64d63e
to
5d6efac
Compare
* Fix amp-ad-3p-impl preconnectCallback * Fixup createIframeWithMessageStub * lint * lint * Simplfy expectMessageFromParent callback
* Fix amp-ad-3p-impl preconnectCallback * Fixup createIframeWithMessageStub * lint * lint * Simplfy expectMessageFromParent callback
3 things:
with multiple slotstest (/cc @tdrl)createIframeWithMessageStubso that exact matches are not necessary (callback can be provided to check/assert the message is the intended one)embed-sizemessages, or if it's not getting the exact right string backamp-ad-3p-impl preconnectCallbacktestCloses #8965 and competes with #8968.